Skip to content

Prevent just a pointerdown on modal close button from closing dialog.#19603

Merged
ornicar merged 4 commits intolichess-org:masterfrom
johndoknjas:require-click-on-close-button-to-close
Feb 27, 2026
Merged

Prevent just a pointerdown on modal close button from closing dialog.#19603
ornicar merged 4 commits intolichess-org:masterfrom
johndoknjas:require-click-on-close-button-to-close

Conversation

@johndoknjas
Copy link
Contributor

To see the current behaviour, open a modal dialog and press down on the close button in its top-right area. This section of the close button is outside the dialog rectangle, so the standard modal behaviour applies (pointerdown outside => close dialog). This is slightly unexpected behaviour though, as it behaves differently from if we press down on the close button in say its bottom-left area.

The motivation came from trying to avoid the resulting behaviour of this use case:

  • Open a study chapter with a number of moves.
  • Scroll down a little and open the help dialog. E.g.:
image - If you then click on the close button in its top-right area, the dialog closes and you're taken to the move in the notation behind it. This is because pointerup on a notation move triggers its selection.

if (e.clientX < r.left || e.clientX > r.right || e.clientY < r.top || e.clientY > r.bottom)
if (
(e.clientX < r.left || e.clientX > r.right || e.clientY < r.top || e.clientY > r.bottom) &&
!dialog.contains(e.target as Node | null) // close button could be positioned outside the dialog
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we still need the X/Y position checks then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ornicar Tested on Chrome & Safari (Mac) and it seems fine to remove them. I don't know enough about modal dialogs to be sure there aren't any edge cases where it'd make a difference though. @jonbgamble any thoughts on this?

@ornicar ornicar merged commit 0e70e06 into lichess-org:master Feb 27, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants